Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use explicit ::godot namespace in gdvirtual.gen.inc #1412

Conversation

dsnopek
Copy link
Collaborator

@dsnopek dsnopek commented Mar 11, 2024

Related to this comment

Will hopefully allow extensions to use the GDVIRTUAL*() macros without requiring using namespace godot

Unfortunately, I don't currently have a project that isn't using using namespace godot to test this with, so I haven't tested if this is sufficient to fix the issue, but it does still build and work on the test project in godot-cpp.

@dsnopek dsnopek added the bug This has been identified as a bug label Mar 11, 2024
@dsnopek dsnopek added this to the 4.3 milestone Mar 11, 2024
@dsnopek dsnopek requested a review from a team as a code owner March 11, 2024 21:24
@Kehom
Copy link
Contributor

Kehom commented Mar 11, 2024

I haven't directly tested the PR yet, however looking at the changes I think there is a bunch of ::godot:: still missing. I have copied the macros that I needed and performed the changes until it's officially fixed. That said, I'm pasting here the const version for one argument with return:

#define GDVIRTUAL1RC(m_ret, m_name, m_type1)\
	::godot::StringName _gdvirtual_##m_name##_sn = #m_name;\
	template <bool required>\
	_FORCE_INLINE_ bool _gdvirtual_##m_name##_call(m_type1 arg1, m_ret &r_ret) const {\
		if (::godot::internal::gdextension_interface_object_has_script_method(_owner, &_gdvirtual_##m_name##_sn)) { \
			::godot::GDExtensionCallError ce;\
			::godot::Variant vargs[1] = { Variant(arg1) };\
			const ::godot::Variant *vargptrs[1] = { &vargs[0] };\
			::godot::Variant ret;\
			::godot::internal::gdextension_interface_object_call_script_method(_owner, &_gdvirtual_##m_name##_sn, (const ::godot::GDExtensionConstVariantPtr *)vargptrs, 1, &ret, &ce);\
			if (ce.error == ::godot::GDEXTENSION_CALL_OK) {\
				r_ret = ::godot::VariantCaster<m_ret>::cast(ret);\
				return true;\
			}\
		}\
		if (required) {\
			ERR_PRINT_ONCE("Required virtual method " + get_class() + "::" + #m_name + " must be overridden before calling.");\
			(void)r_ret;\
		}\
		return false;\
	}\
	_FORCE_INLINE_ bool _gdvirtual_##m_name##_overridden() const {\
		return godot::internal::gdextension_interface_object_has_script_method(_owner, &_gdvirtual_##m_name##_sn); \
	}\
	_FORCE_INLINE_ static ::godot::MethodInfo _gdvirtual_##m_name##_get_method_info() {\
		::godot::MethodInfo method_info;\
		method_info.name = #m_name;\
		method_info.flags = ::godot::METHOD_FLAG_VIRTUAL | ::godot::METHOD_FLAG_CONST;\
		method_info.return_val = ::godot::GetTypeInfo<m_ret>::get_class_info();\
		method_info.return_val_metadata = ::godot::GetTypeInfo<m_ret>::METADATA;\
		method_info.arguments.push_back(::godot::GetTypeInfo<m_type1>::get_class_info());\
		method_info.arguments_metadata.push_back(::godot::GetTypeInfo<m_type1>::METADATA);\
		return method_info;\
	}

As you can see, there are a lot of extra ::godot:: that must be added.

@dsnopek
Copy link
Collaborator Author

dsnopek commented Mar 11, 2024

Some of those things aren't in the godot namespace, namely, anything in the gdextension_interface.h file, which is C, not C++. This includes, for example, GDExtensionCallError and GDEXTENSION_CALL_OK.

@Kehom
Copy link
Contributor

Kehom commented Mar 11, 2024

Interesting. But for some reason here without prefixing those with the godot namespace I got errors (reported by VSCode). I didn't test if it at least compiled without though.

binding_generator.py Outdated Show resolved Hide resolved
@dsnopek dsnopek force-pushed the gdextension-register-virtual-method-namespace branch from fd4dc3f to 14afdc1 Compare March 12, 2024 13:36
@Kehom
Copy link
Contributor

Kehom commented Mar 12, 2024

Hi. I completely messed things up here while attempting to add the last push. Then each attempt to fix things triggered a full godot-cpp rebuild. In other words, it took me some time to finally test things. There is still one missing ::godot::!

There is a block that begins at line 77:

_FORCE_INLINE_ bool _gdvirtual_##m_name##_call($CALLARGS) $CONST {\\
		if (::godot::internal::gdextension_interface_object_has_script_method(_owner, &_gdvirtual_##m_name##_sn)) { \\
			GDExtensionCallError ce;\\
			$CALLSIARGS\\
			Variant ret;\\
			::godot::internal::gdextension_interface_object_call_script_method(_owner, &_gdvirtual_##m_name##_sn, $CALLSIARGPASS, &ret, &ce);\\
			if (ce.error == GDEXTENSION_CALL_OK) {\\
				$CALLSIRET\\
				return true;\\
			}\\
		}\\

Inside of it there is a Variant! I did fix just that line (after refreshing the entire local codebase) and it worked. So I believe it's the last one that must be added.

@dsnopek dsnopek force-pushed the gdextension-register-virtual-method-namespace branch from 14afdc1 to 12ebe4b Compare March 12, 2024 18:07
@dsnopek
Copy link
Collaborator Author

dsnopek commented Mar 12, 2024

Eep, sorry! That last one should be fixed in my latest push.

@Kehom
Copy link
Contributor

Kehom commented Mar 12, 2024

Thank you very much!
Just tested it. Seems to be working!

@dsnopek dsnopek merged commit d78fe98 into godotengine:master Mar 12, 2024
12 checks passed
@dsnopek
Copy link
Collaborator Author

dsnopek commented Mar 12, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This has been identified as a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants